Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

This is where it gets a bit shaky as I have been brainstorming ways to get the available c types in core::ffi to verify the size of the defined types but do not think I have the expertise to do this.

try a program like this:

    println!("{}", std::mem::size_of::<core::ffi::c_long>())

(and so on for the other types.) From there you can parse them into a map like you have for LLVM.

to get a list of the available types you could use something like grep 'type c_[^ ]*' library/core/src/ffi/mod.rs -o | cut -d' ' -f2 | sort -u. on my machine that prints

c_char
c_int
c_long
c_ptrdiff_t
c_size_t
c_ssize_t
c_uint
c_ulong

Oh hey, thanks for working on this!

There are two interesting bits here:

1. There is value in running this for all targets, not just the ones that get native tests run. We can't reasonably build core because of the time that takes.
2. Because of this, we can't actually check what core::ffi::... looks like

What I mentioned in #133058 (comment) works, I was able to prototype this out relatively easily.

Solution to #1

Rust has the internal no_core feature which lets you skip building core and bring only the features you need. It's pretty weird to work with since you need to redefine every little thing you want to use (we provide minicore.rs to help with some of that), but it's pretty useful for running tests. Here, we need to hook a few additional lang items and intrinsics:

// smallcore.rs (within this run-make directory)
#![allow(internal_features)]
#![feature(decl_macro)]
#![feature(intrinsics)]
#![feature(lang_items)]
#![feature(link_cfg)]
#![feature(no_core)]
#![feature(rustc_attrs)]

#![no_core]
#![no_std]

// This is in `tests/auxiliary/minicore.rs`
extern crate minicore;
use minicore::Sized;

// Copy the body from https://github.com/rust-lang/rust/blob/df5b8e39b7def660696340b199ae395a869b3064/library/core/src/internal_macros.rs#L149
macro_rules! cfg_if { /* ... */ }

macro_rules! panic {
    ($msg:literal) => { $crate::panic(&$msg) };
}

/* Intrinsics are function signatures that `rustc` does something magic with.
 * The body doesn't matter. */

#[rustc_intrinsic]
#[rustc_intrinsic_const_stable_indirect]
#[rustc_intrinsic_must_be_overridden]
pub const fn size_of<T>() -> usize {
    loop {}
}

#[rustc_intrinsic]
#[rustc_intrinsic_must_be_overridden]
pub const fn abort() -> ! {
    loop {}
}

/* Lang items are similar to intrinsics but aren't limited to function signatures */

// This is what `panic!()` usually eventually expands to
#[lang = "panic"]
#[rustc_const_panic_str]
const fn panic(_expr: &&'static str) -> ! {
    abort();
}

/* We need to reimplement some basic traits so rustc knows what to do with `==` and `!` */

#[lang = "eq"]
pub trait PartialEq<Rhs: ?Sized = Self> {
    fn eq(&self, other: &Rhs) -> bool;
    fn ne(&self, other: &Rhs) -> bool {
        !self.eq(other)
    }
}

#[lang = "not"]
pub trait Not {
    type Output;
    fn not(self) -> Self::Output;
}

impl PartialEq for usize {
    fn eq(&self, other: &usize) -> bool {
        (*self) == (*other)
    }
}

impl Not for bool {
    type Output = bool;
    fn not(self) -> Self {
        !self
    }
}

(@jieyouxu any interest in just putting some of this in minicore?)

Solution to #2

We can't build core so that means we can't just access the types via core::ffi. This module is simple enough that it can be cleaned up into something that doesn't depend on the rest of core, I just quickly with perl for experimentation but you can just use regex from run_make_support.

# Start with core/src/ffi copied to a temporary directory

# Remove stability attrbutes since they can't be used outside of std
perl -i -0777 -pe 's/#!?\[(un)?stable.*?\]//gs' ffi/mod.rs

# Remove doc attributes since we'll be removing some items they apply to
perl -i -0777 -pe 's/#\[doc.*?\]//gs' ffi/mod.rs

# Remove lang item indicators so they don't conflict if `minicore` gets updated
perl -i -0777 -pe 's/#\[lang.*?\]//gs' ffi/mod.rs

# Remove non-inline modules since we don't need `cstr` or `va_list`
perl -i -0777 -pe 's/.*mod.*;//g' ffi/mod.rs

# Remove reexports that go with those modules
perl -i -0777 -pe 's/.*use.*;//g' ffi/mod.rs

# Remove the `Debug` implementation for `c_void`. This is done in two steps
# to avoid regex tripping over `{`/`}`
perl -i -0777 -pe 's/fn fmt.*?\{.*?\}//gs' ffi/mod.rs
perl -i -0777 -pe 's/impl fmt::Debug for.*?\{.*?\}//gs' ffi/mod.rs

# Just for sanity sake when looking at the generated files, eliminate repeated empty lines
perl -i -0777 -pe 's/\n{3,}/\n\n/gs' ffi/mod.rs

What might actually be better is to move the types we care about into a new mod types, then only the stability attributes need to be stripped (edit: yeah, you should just do this rather than copying my regex hacks).

Tests

Write a test file that asserts each size at compile time, using what we have in smallcore for something like assert_eq! (you could also add an assert_eq macro but 🤷)

// tests.rs

use super::*; // `super` will include everything from `smallcore` once glued together

pub const TEST_C_CHAR_SIZE: () = if size_of::<ffi::c_char>() != CLANG_C_CHAR_SIZE {
    panic!("wrong c_char size");
};

pub const TEST_C_LONG_SIZE: () = if size_of::<ffi::c_long>() != CLANG_C_LONG_SIZE {
    panic!("wrong c_long size");
};

Glue

This is the only target-specific part: each target needs to generate its own file where you fill in the CLANG_* variables based on whatever you collected from Clang.

/* boilerplate part */
#![allow(unused)]

// include! rather than `mod` with `path` since this one has crate-level
// attributes
include!("path/to/smallcore.rs");

// Path to the simplified FFI file
#[path = "path/to/ffi.rs"]
mod ffi;

#[path = "path/to/tests.rs"]
mod tests;

/* end boilerplate */

/* Per-target: append to the template based on Clang's output */

const CLANG_C_CHAR_SIZE: usize = 1;
const CLANG_C_LONG_SIZE: usize = 8;
// ...

Then just have rustc build the file for the relevant target; if our sizes are wrong, we'll get a failure at const eval. You can even pass -Zno-codegen to make this faster (the rustc equivalent of cargo c).

I'll take a look later today, thanks for the detailed writeup.

For checking the sign, you could do add something like this to tests.rs:

trait Signed {
    const SIGNED: bool;
}

impl Signed for i8 {
    const SIGNED: bool = true;
}

impl Signed for u8 {
    const SIGNED: bool = false;
}

const TEST_C_CHAR_SIGNED: () = if ffi::c_char::SIGNED ^ CLANG_C_CHAR_SIGNED {
    panic("mismatched c_char sign");
};

This requires adding a BitXor trait and impl (like Not/PartialEq)

I'll take a look later today, thanks for the detailed writeup.

Thanks! This is less nice than having core available ofc, but this builds a no_core crate for all targets in about a minute on my computer, and it seems reasonably robust.

Most fragile thing is probably the text processing of ffi/mod.rs, but I think we could move everything relevant to ffi/types.rs and just leave a note there not to import anything from crate.

One other thought: this PR shouldn't fix anything that doesn't currently line up, so there should be xfails. It might be easiest to just do this in tests.rs:

// tests that always pass don't need a cfg_if

cfg_if! {
    if #[cfg(target_os = "foobar")] {
        // FIXME: long isn't long enough on foobar
        const XFAIL_C_LONG_SIZE: usize = 16;
        pub const TEST_C_LONG_SIZE: () = if size_of::<ffi::c_long>() != XFAIL_C_LONG_SIZE {
            panic("wrong c_long size");
        };
    } else {
        // Default test
        pub const TEST_C_LONG_SIZE: () = if size_of::<ffi::c_long>() != CLANG_C_LONG_SIZE {
            panic("wrong c_long size");
        };
    }
}

Thanks for the write up, I appreciate it! Will read it and look to understand it tonight :).

Just a quick question that might be off topic but I created the following function for testing purposes:

pub fn create_type_sizes() -> std::collections::HashMap<String, String> {
    let mut sizes = std::collections::HashMap::new();
    sizes.insert("c_char".to_string(), std::mem::size_of::<core::ffi::c_char>().to_string());
    sizes.insert("c_int".to_string(), std::mem::size_of::<core::ffi::c_int>().to_string());
    sizes.insert("c_long".to_string(), std::mem::size_of::<core::ffi::c_long>().to_string());
    sizes.insert("c_ptrdiff_t".to_string(), std::mem::size_of::<core::ffi::c_ptrdiff_t>().to_string());
    sizes.insert("c_size_t".to_string(), std::mem::size_of::<core::ffi::c_size_t>().to_string());
    sizes.insert("c_ssize_t".to_string(), std::mem::size_of::<core::ffi::c_ssize_t>().to_string());
    sizes.insert("c_uint".to_string(), std::mem::size_of::<core::ffi::c_uint>().to_string());
    sizes.insert("c_ulong".to_string(), std::mem::size_of::<core::ffi::c_ulong>().to_string());
    sizes
}

I wanted to save the sizes for the c types for the current target but I get 3 errors for the following types:

c_ptrdiff_t
c_size_t
c_ssize_t

The error is as follows:

use of unstable library feature c_size_t.

The open issue is issue #88345

My concern is that in the tests we are using size_of::ffi::TYPE(). If we input the type as c_size_t or the other 2 we will get an error. Should this effect the run-make test at all or is it fine?

Edit: I am assuming adding #![feature(c_size_t)] allows the run-make test not to be affected by this.

disregard this commit, gonna be following what you outlined for the solution.

I was eepy yesterday, going to revisit this in a moment today. I'll need to revisit what the test requirements are.

Edit: I am assuming adding #![feature(c_size_t)] allows the run-make test not to be affected by this.

Just for reference, this would be correct if importing from core. It shouldn't be relevant though with the no_core solution since the file has to get copied out of core and have the stability attributes stripped.

Solution to #1

Rust has the internal no_core feature which lets you skip building core and bring only the features you need. It's pretty weird to work with since you need to redefine every little thing you want to use (we provide minicore.rs to help with some of that), but it's pretty useful for running tests. Here, we need to hook a few additional lang items and intrinsics:

[... trimmde ...]

(@jieyouxu any interest in just putting some of this in minicore?)

Yes, if these items are only in core (not alloc, not std-only), please do add them to tests/auxiliary/minicore.rs.

Solution to #2

We can't build core so that means we can't just access the types via core::ffi. This module is simple enough that it can be cleaned up into something that doesn't depend on the rest of core, I just quickly with perl for experimentation but you can just use regex from run_make_support.

[... trimmed ...]

What might actually be better is to move the types we care about into a new mod types, then only the stability attributes need to be stripped (edit: yeah, you should just do this rather than copying my regex hacks).

Moving to mod types; sounds less fragile to me, and if the resulting test can properly exercise mod types; through proper name resolution + type checking etc., then please do that instead of regex hacks which will necessarily be fragile.

Glue

This is the only target-specific part: each target needs to generate its own file where you fill in the CLANG_* variables based on whatever you collected from Clang.

/* boilerplate part */
#![allow(unused)]

// include! rather than `mod` with `path` since this one has crate-level
// attributes
include!("path/to/smallcore.rs");

// Path to the simplified FFI file
#[path = "path/to/ffi.rs"]
mod ffi;

#[path = "path/to/tests.rs"]
mod tests;

/* end boilerplate */

/* Per-target: append to the template based on Clang's output */

const CLANG_C_CHAR_SIZE: usize = 1;
const CLANG_C_LONG_SIZE: usize = 8;
// ...

Then just have rustc build the file for the relevant target; if our sizes are wrong, we'll get a failure at const eval. You can even pass -Zno-codegen to make this faster (the rustc equivalent of cargo c).

I don't actually know what -Z no-codegen does, but --emit=metadata is what the check-pass mode does.

@rustbot author

@ricci009 just let us know if you get stuck (absolutely no pressure if it's just a time thing)

@tgross35
Hi, sorry I just got swarmed over this month with holiday and work stuff. I am still working on it and will let you know if there are any issues or I get stuck. Thanks for reaching out :)

@tgross35
I believe I am building the target file incorrectly.

let rustc_output = rustc() .arg("-Z") .arg("unstable-options") .arg("--target") .arg(target) .arg(&file_name) .run();

this is how I attempt to build it but I receive this error:

error[E0463]: can't find crate for std

I have tried to fix this problem through defining #![no_std] in the target file but that does not seem right and does not work.

Smallcore is still the same as what you outlined and I created a mod.rs file like you explained.

I have tried to fix this problem through defining #![no_std] in the target file but that does not seem right and does not work.

That file also needs #![feature(no_core)] and #[no_core]. I'll take a closer look at the rest soon to see if it looks right.

Also as @jieyouxu mentioned above, you can add whatever is missing to tests/auxiliary/minicore.rs and include that rather than creating a new file smallcore.rs (sorry my advice was a bit outdated), so that may make some things simpler.

@bors try

⌛ Trying commit 8fbd055 with merge b5392e7...

💔 Test failed - checks-actions

@rustbot ready

I finally got my hands on an environment where I could properly test with clang. It should pass bors try now @tgross35

I don't know for sure that the LLVM target always matches up with Clang, but that may be something worth trying, considering it hasn't exactly been a 1:1 mapping:

let output = /* command to run rustc --print all-target-specs-json -Z unstable-options */;

let j: Value = serde_json::from_str(output).unwrap();
for (target, v) in j.as_object().unwrap() {
    let llvm_target = &v["llvm-target"].as_str().unwrap();

    // rest of the script
}

@tgross35 interestingly enough I believe this method actually catches even more missalignments between the rust target and what clang is being compiled with. Specifically with os == uefi it matches it to the llvm target in which the os == windows, which leads to a misalignment in signed vs unsigned char, and the size of a long. Thank you for the rec!

Looks like #132570 might fix this discrepancy by using the proper UEFI targets instead of similar windows targets.

@bors delegate+

^ so you can @bors try yourself, just don't merge it until we give a final review

@bors try

✌️ @ricci009, you can now approve this pull request!

If @tgross35 told you to "r=me" after making some further change, please make that change, then do @bors r=@tgross35

@bors try

⌛ Trying commit a88bcbe with merge 52af267...

☀️ Try build successful - checks-actions
Build commit: 52af267 (52af267cd1f6a1a04d832f370ff15648791238c8)

🎉 congrats!

Couldn't have done it without you. Huge thank you for your support :) .

I'd like Jieyou to take a look as well as the rmake expert, which should happen in a few more days. And that gives enough time for the core::ffi refactoring to happen first, if you're able to do that.

Yup gonna start working on this ASAP, I hope to be done with it before Jieyou takes a look at this.

the regex preprocessing would be more robust if the relevant types from core/src/ffi/mod.rs were first moved to library/core/src/ffi/primitives.rs, then there isn't a need to deal with traits / c_str / va_list / whatever might wind up in that module in the future.

Added what the PR should be for reference.